Skip to content

Conversation

@sfc-gh-turbaszek
Copy link
Contributor

@sfc-gh-turbaszek sfc-gh-turbaszek commented Oct 28, 2025

The following code currently won't emit any telemetry:

conn = connect(connection_name="py")
r = conn.execute_string("select 2;")
print(r[0].fetchall())

on the other hand the following code works (sends telemetry)

with connect(connection_name="py") as conn:
    r = conn.execute_string("select 2;")
    print(r[0].fetchall())

The difference is that context manager handles connection closing differently than handler registered atexit. The handler was configured to basically not send telemetry, causing potential loss of observability.

However, the handler is already doing additional REST calls (in majority of cases):

self.rest.delete_session(retry=retry)

which under the hood does:

ret = self._post_request(
url,
headers,
json.dumps(body, cls=SnowflakeRestfulJsonEncoder),
token=self.token,
timeout=5,
no_retry=True,
)

This PR proposes to adopt similar approach (send but not retry) in telemetry close handler.

@sfc-gh-turbaszek sfc-gh-turbaszek requested a review from a team as a code owner October 28, 2025 10:58
@sfc-gh-turbaszek sfc-gh-turbaszek added NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector labels Oct 28, 2025
logger.warning("Failed to add log to telemetry.", exc_info=True)

def send_batch(self) -> None:
def send_batch(self, retry: bool = False) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add tests - to make sure no additional requests are done if retry is False - e.g. using Wiremock or simpler mocks?

method="post",
client=None,
timeout=5,
_no_retry=not retry,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to investigate if the underlying requests library won't execute its built in requests in this case? I think by default there is 1, but we decreased it to 0 for e.g. platform detection.

Fyi HttpConfig can propagate max_retries option there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants